Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add CollectionBuilder attribute to some immutable collection interfaces #89459

Merged
merged 1 commit into from
Aug 9, 2023

Conversation

stephentoub
Copy link
Member

Roslyn asked that these be added. Adding these attributes to the immutable interfaces was discussed as part of #87569 (comment) and left up to the working group to decide what to do with it, which is this PR.

@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost ghost assigned stephentoub Jul 25, 2023
@ghost
Copy link

ghost commented Jul 25, 2023

Tagging subscribers to this area: @dotnet/area-system-collections
See info in area-owners.md if you want to be subscribed.

Issue Details

Roslyn asked that these be added. Adding these attributes to the immutable interfaces was discussed as part of #87569 (comment) and left up to the working group to decide what to do with it, which is this PR.

Author: stephentoub
Assignees: -
Labels:

area-System.Collections, new-api-needs-documentation

Milestone: 8.0.0

@CyrusNajmabadi
Copy link
Member

@cston @RikkiGibson @captainsafia . We shoudl have a meeting to conclude on this. If we do take this path, we will need to ensure the lang spec/impl allows for this. Specifically, where the factory method returns a different (but implicit-reference compatible) result.

@RikkiGibson
Copy link
Contributor

Totally. I don't know whether/how we should limit the permitted implicit conversions. For example, if the implementation type is a struct, is it ok to do a boxing conversion here? The user is applying the attribute. We might assume they know which method is being referenced in the attribute and they're OK with the semantics of converting the result to interface type.

@CyrusNajmabadi
Copy link
Member

Totally. I don't know whether/how we should limit the permitted implicit conversions. For example, if the implementation type is a struct, is it ok to do a boxing conversion here? The user is applying the attribute. We might assume they know which method is being referenced in the attribute and they're OK with the semantics of converting the result to interface type.

My presumptino is that at least an implicit conversion needs to exist.

  1. i don't believe we need to validate that form them at the declaration site.
  2. at teh use site, i think we should just treat this like assignment and only allow if hte assignment is allowed. we can report an error if not (either that no compatible method was found, or specifically that hte return type has no viable conversion).

In practice, i expect usage to be exactly like this. There is a concrete type returned and it has an implicit conversion (ideally reference, but any implicit seems fine) to the interface. I think boxing is fine as i could totally see a lightweight struct being an impl for some types, and the interface just going: yup, good enough for me.

@stephentoub stephentoub added the blocked Issue/PR is blocked on something - see comments label Jul 26, 2023
@stephentoub stephentoub added NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) and removed NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) blocked Issue/PR is blocked on something - see comments labels Aug 7, 2023
@stephentoub
Copy link
Member Author

C# 12 is going to support the conversion.

@stephentoub stephentoub merged commit a67d5f1 into dotnet:main Aug 9, 2023
@stephentoub stephentoub deleted the immutableinterfacecreate branch August 9, 2023 19:04
@ghost ghost locked as resolved and limited conversation to collaborators Sep 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants